Merged
Conversation
- forecast_selectors: raise ValueError if season or col_name is None - tile_url_callback: also check issue_month_abbrev before PreventUpdate
There was a problem hiding this comment.
Pull request overview
This pull request adds a Start Year control to the FBF maproom that allows users to set the earliest year for table data display. The changes include UI components, callback logic, and API support for the new functionality.
Changes:
- Added a new "Start year" dropdown control to the table layout that filters table data to show only years from the selected start year onwards
- Updated the export API to accept an optional start_year query parameter with proper defaulting to season configuration
- Added validation checks to prevent errors when controls are unset (None checks in forecast_selectors and tile_url_callback)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| fbfmaproom/fbflayout.py | Added new Start Year dropdown control to the table controls section |
| fbfmaproom/fbfmaproom.py | Added start_year_selector callback, updated table_cb and generate_tables to use start_year parameter, added None validation to existing callbacks, and updated export_endpoint to support optional start_year parameter |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Collaborator
aaron-kaplan
left a comment
aaron-kaplan
left a comment
There was a problem hiding this comment.
Looks good except that you haven't added start_year to the query string, so the value won't be saved in the URL for bookmarking and sharing the way the other controls are. For writing the value to the query string, see where it says APP.clientside_callback (JavaScript in a Python string, yuck), and for reading from the querystring into the application, see any place where State("location", "search") is used.
Other than that, my comments below are mostly for my edification, not necessarily saying what you did was wrong. It's just that I see changes in several places that seem unrelated to the start_year functionality, so I'd like to know what problems you're fixing there.
Contributor
Author
|
Most of them are not necessary at all, just me throwing them in there to
understand the behavior of the app. Especially when the app first loads and
the callback runs before the season / map_column / issue_month dropdowns
are filled or selected.
We can remove them. I'll clean them out when I update the code for the
query string.
…On Tue, Jan 20, 2026 at 4:36 PM Aaron Kaplan ***@***.***> wrote:
@ aaron-kaplan commented on this pull request. Looks good except that you
haven't added start_year to the query string, so the value won't be saved
in the URL for bookmarking and sharing the way the other controls are. For
writing the value to
ZjQcmQRYFpfptBannerStart
This Message Is From an External Sender
This message came from outside your organization.
ZjQcmQRYFpfptBannerEnd
***@***.**** commented on this pull request.
Looks good except that you haven't added start_year to the query string,
so the value won't be saved in the URL for bookmarking and sharing the way
the other controls are. For writing the value to the query string, see
where it says APP.clientside_callback (JavaScript in a Python string,
yuck), and for reading from the querystring into the application, see any
place where State("location", "search") is used.
Other than that, my comments below are mostly for my edification, not
necessarily saying what you did was wrong. It's just that I see changes in
several places that seem unrelated to the start_year functionality, so I'd
like to know what problems you're fixing there.
------------------------------
In fbfmaproom/fbfmaproom.py
<https://urldefense.com/v3/__https://github.com/iridl/python-maprooms/pull/587*discussion_r2710099686__;Iw!!BDUfV1Et5lrpZQ!TxPhP8YaMIf_f-AN59gPKp2flQ14B_UCj0qU5qVfC4dxOkDAaA_OAnOrMznAIW8ouWGlqVoG-Ch-0B9uJNiKU6pQ2p8V2g$>
:
> @@ -959,6 +960,8 @@ def custom_static(relpath):
)
def forecast_selectors(season, col_name, pathname, qstring):
try:
+ if season is None or col_name is None:
+ raise ValueError("Season or column name is None")
Why did you add this, and why for only two of the parameters?
------------------------------
In fbfmaproom/fbfmaproom.py
<https://urldefense.com/v3/__https://github.com/iridl/python-maprooms/pull/587*discussion_r2710104176__;Iw!!BDUfV1Et5lrpZQ!TxPhP8YaMIf_f-AN59gPKp2flQ14B_UCj0qU5qVfC4dxOkDAaA_OAnOrMznAIW8ouWGlqVoG-Ch-0B9uJNiKU6pJXZiAWQ$>
:
> @@ -1017,6 +1020,47 @@ def forecast_selectors(season, col_name, pathname, qstring):
)
***@***.***(
+ Output("start_year", "options"),
+ Output("start_year", "value"),
+ Input("season", "value"),
+ Input("location", "pathname"),
+ State("location", "search"),
+)
+def start_year_selector(season, pathname, qstring):
+ try:
+ if season is None:
+ raise ValueError("Season is None")
Again, why are you checking for None in some parameters but not others?
------------------------------
In fbfmaproom/fbfmaproom.py
<https://urldefense.com/v3/__https://github.com/iridl/python-maprooms/pull/587*discussion_r2710112633__;Iw!!BDUfV1Et5lrpZQ!TxPhP8YaMIf_f-AN59gPKp2flQ14B_UCj0qU5qVfC4dxOkDAaA_OAnOrMznAIW8ouWGlqVoG-Ch-0B9uJNiKU6rjiZUbUw$>
:
> +
+ start_year_options = [
+ dict(label=str(year), value=year)
+ for year in year_range
+ ]
+
+ start_year_value = parse_arg(
+ "start_year",
+ conversion=int,
+ default=year_min,
+ qstring=qstring
+ )
+ except Exception:
+ traceback.print_exc()
+ start_year_options = dash.no_update
+ start_year_value = dash.no_update
Why did you add this exception handler? Isn't this the same as the default
behavior, i.e. the same as what would happen if you deleted the try/except?
------------------------------
In fbfmaproom/fbfmaproom.py
<https://urldefense.com/v3/__https://github.com/iridl/python-maprooms/pull/587*discussion_r2710119672__;Iw!!BDUfV1Et5lrpZQ!TxPhP8YaMIf_f-AN59gPKp2flQ14B_UCj0qU5qVfC4dxOkDAaA_OAnOrMznAIW8ouWGlqVoG-Ch-0B9uJNiKU6pDnFLYKg$>
:
> @@ -1201,7 +1247,7 @@ def update_severity_color(value):
)
def tile_url_callback(target_year, issue_month_abbrev, freq, pathname, map_col_key, season_id):
colorscale = None # default value in case an exception is raised
- if season_id is None:
+ if season_id is None or issue_month_abbrev is None:
Why this change?
------------------------------
In fbfmaproom/fbfmaproom.py
<https://urldefense.com/v3/__https://github.com/iridl/python-maprooms/pull/587*discussion_r2710148485__;Iw!!BDUfV1Et5lrpZQ!TxPhP8YaMIf_f-AN59gPKp2flQ14B_UCj0qU5qVfC4dxOkDAaA_OAnOrMznAIW8ouWGlqVoG-Ch-0B9uJNiKU6pferCssg$>
:
> +
+ start_year_value = parse_arg(
+ "start_year",
+ conversion=int,
+ default=year_min,
+ qstring=qstring
+ )
+ except Exception:
+ traceback.print_exc()
+ start_year_options = dash.no_update
+ start_year_value = dash.no_update
+
+ return (
+ start_year_options,
+ start_year_value,
+ )
Initial values for most of the variables are set in initial_setup. I
think I would have added this logic to that function instead of making a
separate callback. Is there a reason to do it this way?
—
Reply to this email directly, view it on GitHub
<https://urldefense.com/v3/__https://github.com/iridl/python-maprooms/pull/587*pullrequestreview-3684328271__;Iw!!BDUfV1Et5lrpZQ!TxPhP8YaMIf_f-AN59gPKp2flQ14B_UCj0qU5qVfC4dxOkDAaA_OAnOrMznAIW8ouWGlqVoG-Ch-0B9uJNiKU6qZ9Ze7Lg$>,
or unsubscribe
<https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/AFK3DJK7RNAEHX73LKD3YMD4H2NXHAVCNFSM6AAAAACSKLI7F6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTMOBUGMZDQMRXGE__;!!BDUfV1Et5lrpZQ!TxPhP8YaMIf_f-AN59gPKp2flQ14B_UCj0qU5qVfC4dxOkDAaA_OAnOrMznAIW8ouWGlqVoG-Ch-0B9uJNiKU6pHvxvFuQ$>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
--
Best,
*Nitin Magima (he/him/his) *
Staff Associate II, Officer of Research
MPA DP | FSA Credential Holder | AWS Certified Cloud Practitioner
National Center for Disaster Preparedness (NCDP) <http://ncdp.columbia.edu>
| Affiliate of IRI
Columbia Climate School | Columbia University
Office: 212.853.NCDP
E-mail: ***@***.***
Medium ***@***.***> | Github
<https://github.com/nitinmagima> | LinkedIn
<https://www.linkedin.com/in/nitin-magima/>
|
Contributor
Author
|
Updated the URL Query string as well. It shows up in the URL, so I think it works! |
Collaborator
What you need to test is not only that it shows up in the URL, but that when you reload that URL, or bookmark it and then come back to it in a new window, the setting is applied. I checked, and it is, so good job! Set up a test server and share it with the group. If there are no comments from the others, I will merge. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Added a Start Year control to the FBF maproom so we can set the earliest year for the table, and updated the export API and callback validation to support it and avoid errors when controls are unset.
Changes
Optional start_year query parameter (int): earliest year for exported skill, history, and threshold. If omitted, defaults to the season’s start_year from config.